-
Notifications
You must be signed in to change notification settings - Fork 101
Embedded OTel Collector Config #1314
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
config_path: "/etc/nginx-agent/nginx-agent-otelcol.yaml" | ||
additional_config_paths: | ||
- "/configs/my_config.yaml" | ||
- /etc/nginx-agent/nginx-agent-otelcol.yaml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be commented?
- /etc/nginx-agent/nginx-agent-otelcol.yaml | |
- "/etc/nginx-agent/nginx-agent-otelcol.yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's without the quotes because I wanted to check it would work with and without the quotes
return fmt.Errorf("error while marshaling to YAML: %w", err) | ||
} | ||
|
||
writeErr := os.WriteFile("/var/lib/nginx-agent/opentelemetry-collector-agent-debug.yaml", b, filePermission) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you make "/var/lib/nginx-agent/opentelemetry-collector-agent-debug.yaml" a constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it be better to write the file to the location of the default YAML file if it is present?
Something like:
newFileName := "opentelemetry-collector-agent-debug.yaml"
folderPath := filepath.Dir(CollectorConfigPathKey)
DebugFilePath := filepath.Join(folderPath, newFileName)
os.WriteFile(DebugFilePath)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the ask was to put it in var/lib/nginx-agent as the user should not edit this file and that is usually where we place things which we do not want to user to modify
internal/collector/settings.go
Outdated
return []string{cfg.Collector.ConfigPath} | ||
configFiles := []string{cfg.Collector.ConfigPath} | ||
configFiles = slices.Concat(configFiles, cfg.Collector.AdditionalPaths) | ||
slog.Info("Additional config files:", "", configFiles) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
slog.Info("Additional config files:", "", configFiles) | |
slog.Info("Merging additional OTel config files", "config_files", configFiles) |
internal/config/types.go
Outdated
Pipelines Pipelines `yaml:"pipelines" mapstructure:"pipelines"` | ||
Receivers Receivers `yaml:"receivers" mapstructure:"receivers"` | ||
ConfigPath string `yaml:"config_path" mapstructure:"config_path"` | ||
AdditionalPaths []string `yaml:"additional_config_paths" mapstructure:"additional_config_paths"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AdditionalPaths []string `yaml:"additional_config_paths" mapstructure:"additional_config_paths"` | |
AdditionalConfigPaths []string `yaml:"additional_config_paths" mapstructure:"additional_config_paths"` |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1314 +/- ##
==========================================
+ Coverage 84.88% 84.92% +0.04%
==========================================
Files 100 100
Lines 14520 14566 +46
==========================================
+ Hits 12325 12370 +45
+ Misses 1734 1728 -6
- Partials 461 468 +7
... and 5 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
Can you add unit tests for the new functions including all potential error paths? |
Proposed changes
This change allows a user to list paths to their own Otel configs, these are then merged with the default Agent collector config. The last config specified takes priority over the rest if they have the same values declared.
To make debugging easier the merged OTel config which OTel is running with is outputted to
/var/lib/nginx-agent/opentelemetry-collector-agent-debug.yaml
To make modifying the NGINX Receiver and NGINX Plus Receiver easier if there is only one instance running thee instance ID is no longer needed be needed for the name. This will make modifying the
collection_interval
setting for example using your own OTel config easier as you no longer need the instanceIDwill now be
Checklist
Before creating a PR, run through this checklist and mark each as complete.
CONTRIBUTING
documentmake install-tools
and have attached any dependency changes to this pull requestREADME.md
)